Remove dropwizard-jackson dep from core#435
Conversation
polaris-core/src/main/java/org/apache/polaris/core/persistence/MetaStoreManagerFactory.java
Outdated
Show resolved
Hide resolved
|
I'm going to add one more commit and rebase presently... hopefully clarifying the use of |
0bb41e3 to
fd87402
Compare
|
I rebased and refactored service descriptors (resources) to be coherent both with java type hierarchies and comply with the Dropwizard way of discovering Jackson sub-types. As a side benefit, the last commit fixes this message that happens when running without EclipseLink: |
adutra
left a comment
There was a problem hiding this comment.
LGTM – I think you achieved a nice balance between removing DW from core but keeping Discoverable around in relevant modules. 👍
There was a problem hiding this comment.
This doesn't make a lot of sense to me. When the subtype resolver is asked for implementations, it'll be for a common interface - i.e., MetaStoreManagerFactory in this case. Why would the subtype resolver ever ask for the eclipse-link specific version of the DiscoverableMetaStoreManagerFactory?
There was a problem hiding this comment.
Type resolution during YAML parsing is handled by Jackson, and it properly recognizes all types in the java hierarchy (even those not implementing Discoverable).
However, Jackson (in this setup) relies on DiscoverableSubtypeResolver to discover implementation classes. That latter part has peculiar logic, where DiscoverableSubtypeResolver performs a two stage lookup: 1) classes listed in META-INF/services/io.dropwizard.jackson.Discoverable, 2) classes listed in the service manifests for the classes from step 1. I do not really know why it does not directly inject classes from step 1 into Jackson.
I agree, it looks odd. However, having one Discoverable interface is not possible as this PR specifically proposes to remove DW dependencies from polaris-core, where the base interface exists and there is no other common root between the "in memory" metastore implementation and the EclipseLink implementation. We could add a rather small common module for that, of course, but in my mind having two separate Discoverable interfaces was less intrusive. WDYT?
There was a problem hiding this comment.
Leaf classes cannot implement {@link Discoverable} directly, they have to implement another interface, which implements {@link Discoverable}.
Isn't this the exact setup we have right now, where the leaf classes implement MetastoreManagerFactory?
Should we just move MetastoreManagerFactory out of core?
There was a problem hiding this comment.
Should we just move MetastoreManagerFactory out of core?
That's an option. This is what I meant by "add a rather small common module" above, because the EclipseLink and In-Memory implementations have to share it.
Would do other people think? @collado-mike @adutra ?
There was a problem hiding this comment.
So, if I get this correctly, we would all love to have MetastoreManagerFactory directly in polaris-service like many other similar beans, but we can't, because of EclipseLink.
If that is correct, I'd prefer to keep your current solution for now, then revisit this later, when/if EclipseLink is replaced with some more robust persistence solution.
There was a problem hiding this comment.
(and in any case, I'd avoid introducing a "polaris-core-dropwizard" module just for that.)
This is a simplification / cleanup. The dependency does not appear to be required in `polaris-core`
fd87402 to
c343504
Compare
|
Alternative implementation: #470 |
|
Closing in favour of #470 |
This is a simplification / cleanup. The dependency does not appear to be required in
polaris-coreHow Has This Been Tested?
Manual
bootstrapexecution with EclipseLink and PostgreSQL.